-
Notifications
You must be signed in to change notification settings - Fork 7.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
engine: update chmod command for gpg keys on debian #17070
Conversation
✅ Deploy Preview for docsdocker ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
8b5074d
to
2d244d0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a comment (we may have to check what we do in other examples)
engine/install/debian.md
Outdated
> $ sudo chmod a+r /etc/apt/keyrings/docker.gpg | ||
> $ sudo chmod -R a+rx /etc/apt/keyrings |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this may be a bit too eager;
mkdir keyrings
touch keyrings/one.sh
touch keyrings/docker.gpg
ls -al keyrings/
total 8
drwxr-xr-x 2 root root 4096 Apr 8 11:41 .
drwxr-xr-x 3 root root 4096 Apr 8 11:41 ..
-rw-r--r-- 1 root root 0 Apr 8 11:41 docker.gpg
-rw-r--r-- 1 root root 0 Apr 8 11:41 one.sh
Notice that all files in the directory now have their executable bit set (including files we don't own);
chmod -R a+rx keyrings
ls -l keyrings/
total 8
drwxr-xr-x 2 root root 4096 Apr 8 11:41 .
drwxr-xr-x 3 root root 4096 Apr 8 11:41 ..
-rwxr-xr-x 1 root root 0 Apr 8 11:41 docker.gpg
-rwxr-xr-x 1 root root 0 Apr 8 11:41 one.sh
I think we need two steps;
- one to set the executable bit on the directory to make it browsable (using capital
X
to only touch the directory itself) - one to set the permissinos on the
docker.gpg
(we should not touch other files that we don't own)
mkdir keyrings2
touch keyrings2/one.sh
touch keyrings2/docker.gpg
chmod -R a+rX keyrings2
chmod -R a+r keyrings2/docker.gpg
ls -al keyrings2/
total 8
drwxr-xr-x 2 root root 4096 Apr 8 11:41 .
drwxr-xr-x 4 root root 4096 Apr 8 11:41 ..
-rw-r--r-- 1 root root 0 Apr 8 11:41 docker.gpg
-rw-r--r-- 1 root root 0 Apr 8 11:41 one.sh
I'm actually wondering, perhaps we could update the install steps to use Here, I mimic a situation where the directory already exists with 0700 perms, and use mkdir -m 0700 keyrings
ls -l
total 4
drwx------ 2 root root 4096 Apr 8 11:54 keyrings
mkdir -m 0755 -p keyrings
ls -l
total 4
drwx------ 2 root root 4096 Apr 8 11:54 keyrings
ls -l
total 8
drwx------ 2 root root 4096 Apr 8 11:54 keyrings
install -m 0755 -d keyrings
ls -l
total 8
drwxr-xr-x 2 root root 4096 Apr 8 11:54 keyrings
install -m 0755 -d keyrings2
ls -l
total 8
drwxr-xr-x 2 root root 4096 Apr 8 11:54 keyrings
drwxr-xr-x 2 root root 4096 Apr 8 11:55 keyrings2 |
I think with the |
Signed-off-by: David Karlsson <david.karlsson@docker.com>
2d244d0
to
7ad91f1
Compare
Nice, I like it. I had a go at updating to use Please take a look! |
I didn't know about it until "not too long ago", when I stumbled upon it when working on some of our packaging scripts. It's definitely not a command I think of "all the time", but it's pretty useful actually! |
> Receiving a GPG error when running `apt-get update`? | ||
> | ||
> Your default [umask](https://en.wikipedia.org/wiki/Umask){: target="blank" | ||
> rel="noopener" } may be incorrectly configured, preventing detection of the | ||
> repository public key file. Try granting read permission for the Docker | ||
> public key file before updating the package index: | ||
> | ||
> ```console | ||
> $ sudo chmod a+r /etc/apt/keyrings/docker.gpg | ||
> $ sudo apt-get update | ||
> ``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
before we fully celebrate, let's also get some more eyes on this in case we're overlooking scenarios where someone could still run into this.
Overall I think we should be fine (assuming users just ran the steps above), and I guess if someone ran those steps with the old variant, they would already have run into the issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh! 🤦 I think there is still a situation; we now fixed the directory permissions, but we don't explicitly set read permissions on the docker.gpg
itself.
I guess we could either keep a note (but I hate "too many notes" as they distract from the overall flow), or we could add a line to always set the permissions;
$ curl -fsSL {{ download-url-base }}/gpg | sudo gpg --dearmor -o /etc/apt/keyrings/docker.gpg
$ sudo chmod a+r /etc/apt/keyrings/docker.gpg
I double-checked and yes, looks like we still may have to (here's with a 0066
umask);
umask 0066
curl -fsSL https://download.docker.com/linux/ubuntu/gpg | gpg --dearmor -o /etc/apt/keyrings/docker2.gpg
ls -l /etc/apt/keyrings/
total 8
-rw-r--r-- 1 root root 2760 Jun 7 2022 docker.gpg
-rw------- 1 root root 2760 Apr 8 16:20 docker2.gpg
umask 0022
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added chmod
to the previous step - I agree that too many notes creates clutter and makes it hard to read
Signed-off-by: David Karlsson <david.karlsson@docker.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@tianon @neersighted perhaps you want to give this a sanity check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems reasonable to me -- I worried for a moment about users who might have had different permissions on /etc/apt/keyrings
on purpose, but those users aren't likely to be blindly copy/pasting that install
line from these docs (just recording the thought process 👍).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lost this in my notifications; seems reasonable to me as well/matches what I do on my systems.
Proposed changes
Related issues (optional)
Follow-up: #16674